Fix exception thrown in CLI when bicepconfig.json is invalid #4348
Conversation
| var compilation = new Compilation(this.invocationContext.ResourceTypeProvider, sourceFileGrouping); | ||
| try | ||
| { | ||
| var compilation = new Compilation(this.invocationContext.ResourceTypeProvider, sourceFileGrouping); |
There was a problem hiding this comment.
Basically the reasoning is that the user can get much of the functionality even with a malformed config file, so the compilation shouldn't just give up.
There was a problem hiding this comment.
Like I mentioned in the below comment, Compilation needs a valid config. If we encounter an invalid config, should we surface exception to user, create default disabled linter config and proceed with creating compilation?
There was a problem hiding this comment.
Created below issue to track the issue on language server side:
#4471
There was a problem hiding this comment.
Made changes to write error message and go ahead with compilation creation:
699af2a
|
|
||
| return compilation; | ||
| } | ||
| catch (Exception exception) |
There was a problem hiding this comment.
Hmmm, we do this in language server as well:
On the language server side, if the config file is invalid, we don't proceed till config file is error free. The current compilation design needs a config file.
What do we want the behavior to be? If we encounter invalid config file, log diagnostic and use default config settings? That would be confusing.
One option I can think of:
- If the config file is invalid, log error diagnostic
- Use disabled config, something similar to what we do in tests to create compilation
- Push other diagnostics along with the error from invalid config
Let me know your thoughts. Adding @anthony-c-martin as well.
There was a problem hiding this comment.
We should definitely surface the exception to the user, but until now, we've just relied on the try-catch in Program.RunAsync for this. Is there a reason to write a diagnostic instead of following this pattern?
There was a problem hiding this comment.
Do you mean use InvocationContext to write error? That would look like this:
There was a problem hiding this comment.
Yeah - in the above screenshot, the exception message itself is definitely useful, but I don't think there's any need to log the stack trace. The only thing I'd change in the message is making sure we log which file couldn't be parsed - so the user knows where to look for the bad configuration file.
There was a problem hiding this comment.
Created below issue to track the issue on language server side:
#4471
There was a problem hiding this comment.
Made changes to write error message and go ahead with compilation creation:
699af2a
bhsubra
force-pushed
the
dev/bhsubra/Fix4242
branch
from
d9c342b to
699af2a
Compare
Fixes #4242
Without changes in this PR, with invalid bicepconfig.json, we throw unhandled exception:
As part of this PR, made changes to handle the exception gracefully
